Skip to content

Conversation

@TkDodo
Copy link
Contributor

@TkDodo TkDodo commented Oct 4, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 4, 2025

@TkDodo is attempting to deploy a commit to the 47ng Team on Vercel.

A member of the Team first needs to authorize it.

@franky47 franky47 added the deploy:preview Deploy a preview version of this PR on pkg.pr.new label Oct 5, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 5, 2025

pnpm add https://pkg.pr.new/nuqs@1159

commit: 77b09aa

Copy link
Contributor Author

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I’m still seeing an additional re-render after writing the default to the url, even though we now bail out after calling setState(v => v).

I think that’s because the component still has null in its internal cache, and the useEffect updates it to the initial value.

This is not ideal because it will always happen when writeDefaults is set, even if you already have a value in the url.

Maybe you have a better idea how to write a value to the url without triggering any subscribers ?

// effect to write defaults to the url on mount
useEffect(() => {
if (optionWriteDefaults || anyParserWriteDefaults) {
void update(s => s)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what updates the url and puts the defaults in there. Should we make this force to history: 'replace' ? I don't see how a push would make any sense here because if you try to navigate back, it would just automatically push the default in there again...

Copy link
Member

@franky47 franky47 Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah writeDefaults should use replace, that makes more sense.

As for "not triggering any subscribers" in #1159 (review), that depends on the frameworks, but generally when the URL changes, their useSearchParams API or equivalent will re-render at least once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are fine with the tradeoff, I am too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about at least bailing out of writing to the url if the url is empty and we want to write the defaultValue to it, but then again, scheduling makes this tricky: what if there’s another write scheduled, then we can’t just opt-out of this...

Copy link
Member

@franky47 franky47 Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like maybe setState(v => v) is too high-level an approach for this, maybe this needs to be better integrated with the throttle queue state.

The throttle queue works in two phases:

  1. Collecting pushes (in either the same event loop tick for sync batching, or until the throttle time expires if there has been a flush recently), which merges update requests into a map as it goes along.
  2. A request for flush is made, which schedules it according to the queue's timing state (if enough time has passed, it gets flushed on next tick, otherwise at the end of the throttle timeout).

The debounce queues also offload their output to the throttle queue, so all updates that need to reach the URL go through the same pipeline.
The optimistic state and returned Promise flows the other way around, from the throttle queue to the debounce controller.

We could read from the adapter's getSearchParamsSnapshot for the "unoptimistic" view of what the URL is like for the router, and read the queued updates directly from the debounce controller (like useQueuedQueries, but in a non-hook, one-shot way), then decide whether or not to push+flush the update.

@TkDodo TkDodo marked this pull request as ready for review October 7, 2025 11:47
* Set it to `false` to keep backwards-compatiblity when the default value
* changes (prefer explicit URLs whose meaning don't change).
*
* @deprecated use `writeDefaults` instead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: we should add a migration note here that the boolean logic is inverted: clearOnDefault: falsewriteDefaults: true.

Comment on lines +401 to +403
const anyParserWriteDefaults = Object.values(keyMap).some(
v => v.writeDefaults
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: What happens in the following (albeit contrived) scenario?

const [dynamic, setDynamic] = useState(false)
useQueryStates({
  a: parseAsString.withDefault('a').withOptions({ writeDefaults: false }),
  b: parseAsString.withDefault('b').withOptions({ writeDefaults: dynamic })
})
// Then toggle dynamic

I would expect a never to have its default being applied to the URL (as it's asking not to), but it feels like dynamic becoming true would trigger the effect and write them both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deploy:preview Deploy a preview version of this PR on pkg.pr.new

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants